helm & e2e: add option to enable read affinity for rbd #4111

iPraveenParihar · 2023-09-07T05:17:01Z

Describe what this PR does

helm: add option to enable read affinity for rbd
This commit adds --enable-read-affinity flag to
enable read affinity for rbd
e2e: added test to verify read affinity functionality
e2e test case is added to test if read affinity is enabled by
verifying read_from_replica=localize option is passed.

Closes: #3642

Rakshith-R · 2023-09-07T05:32:32Z

charts/ceph-csi-rbd/values.yaml

+    - topology.rook.io/datacenter
+    - topology.rook.io/room
+    - topology.rook.io/pod
+    - topology.rook.io/pdu
+    - topology.rook.io/row
+    - topology.rook.io/rack
+    - topology.rook.io/chassis


We can just have the above labels as examples.
These rook specific labels can be removed.

Madhu-1 · 2023-09-07T06:42:04Z

charts/ceph-csi-rbd/templates/nodeplugin-daemonset.yaml

+{{- if .Values.readAffinity.enabled }}
+            - "--enable-read-affinity=true"


This will be problematic if we choose to enable it by default later for some reason and the user should be able to disable it

@Madhu-1, I didn't get you on this. Can you explain bit more?

Suggested change

{{- if .Values.readAffinity.enabled }}

- "--enable-read-affinity=true"

- "--enable-read-affinity={{ .Values.readAffinity.enabled }}"

{{- if .Values.readAffinity.enabled }}

- "--crush-location-labels={{ .Values.readAffinity.crushLocationLabels | join "," }}"

{{- end }}

The above change will allow users to disable the ReadAffinity if its get enabled by default.

Okay, I got it
Thanks!

Madhu-1 · 2023-09-07T06:43:50Z

charts/ceph-csi-rbd/README.md

@@ -176,6 +176,8 @@ charts and their default values.
 | `provisioner.podSecurityPolicy.enabled`        | Specifies whether podSecurityPolicy is enabled                                                                                                       | `false`                                            |
 | `topology.enabled`                             | Specifies whether topology based provisioning support should be exposed by CSI                                                                       | `false`                                            |
 | `topology.domainLabels`                        | DomainLabels define which node labels to use as domains for CSI nodeplugins to advertise their domains                                               | `{}`                                               |
+| `readAffinity.enabled` | Enable read affinity for RBD volumes. Recommended to set to true if running kernel 5.8 or newer. | `false` |
+| `readAffinity.crushLocationLabels` | Define which node labels to use as CRUSH location. This should correspond to the values set in the CRUSH map. | labels listed [here](https://github.com/rook/rook/blob/master/Documentation/CRDs/Cluster/ceph-cluster-crd.md#osd-topology) |


Can you point to the ceph doc instead of the Rook doc for this one? if not, this can never be used with regular ceph clusters?

Praveen, The default is empty list [],

add a sentence "For more information, click [here]"
https://github.com/ceph/ceph-csi/blob/v3.9.0/docs/deploy-rbd.md#read-affinity-using-crush-locations-for-rbd-volumes

Users can refer to this section for more details

@Rakshith-R, I have made the changes

iPraveenParihar · 2023-09-14T08:29:41Z

e2e/deployment.go

+
+	// enable read affinity support (for RBD)
+	enableReadAffinity  bool
+	crushLocationLabels string


Hi @Madhu-1,
golangci-lint is faling because of maligned struct

e2e/deployment.go:233:29: struct of size 80 bytes could be of size 72 bytes: struct{ filename string, namespace string, domainLabel string, crushLocationLabels string, oneReplica bool, enableTopology bool, enableReadAffinity bool, } ``` (maligned) type yamlResourceNamespaced struct {

Do we reorder as suggested?

yes it's best to do it as suggested.

iPraveenParihar · 2023-09-14T12:19:28Z

/test ci/centos/mini-e2e/k8s-1.26

iPraveenParihar · 2023-09-14T14:30:06Z

/test ci/centos/mini-e2e-helm/k8s-1.26

iPraveenParihar · 2023-09-14T14:41:08Z

/test ci/centos/mini-e2e-helm/k8s-1.27

iPraveenParihar · 2023-09-14T14:41:19Z

/test ci/centos/mini-e2e/k8s-1.27

iPraveenParihar · 2023-09-14T14:41:54Z

/test ci/centos/mini-e2e/k8s-1.28

iPraveenParihar · 2023-09-14T14:42:04Z

/test ci/centos/mini-e2e-helm/k8s-1.28

iPraveenParihar · 2023-09-14T16:47:09Z

@Rakshith-R,
For the e2e test case - verifying just the presence of the read_from_replica=localize option is enough?
As the crush location label:value will be in random order.

example - crush_location=zone:east-zone1|region:east or it may be crush_location=region:east|zone:east-zone1.
In this case, I'll have to parse thecrush_location value and then verify each label:value individually

Rakshith-R · 2023-09-15T10:29:53Z

@Rakshith-R, For the e2e test case - verifying just the presence of the read_from_replica=localize option is enough? As the crush location label:value will be in random order.

example - crush_location=zone:east-zone1|region:east or it may be crush_location=region:east|zone:east-zone1. In this case, I'll have to parse thecrush_location value and then verify each label:value individually

It'll be great to parse and verify the key value pairs too.

iPraveenParihar · 2023-09-19T04:27:06Z

/test ci/centos/mini-e2e/k8s-1.26

iPraveenParihar · 2023-09-19T04:29:18Z

/test ci/centos/mini-e2e/k8s-1.27

iPraveenParihar · 2023-09-19T04:29:31Z

/test ci/centos/mini-e2e/k8s-1.28

iPraveenParihar · 2023-09-19T06:44:59Z

/test ci/centos/mini-e2e/k8s-1.28

iPraveenParihar · 2023-09-19T11:18:03Z

/test ci/centos/mini-e2e-helm/k8s-1.26

iPraveenParihar · 2023-09-19T11:18:08Z

/test ci/centos/mini-e2e-helm/k8s-1.27

iPraveenParihar · 2023-09-19T11:18:15Z

/test ci/centos/mini-e2e-helm/k8s-1.28

This commit adds --enable-read-affinity flag to enable read affinity for rbd Signed-off-by: Praveen M <m.praveen@ibm.com>

e2e test case is added to test if read affinity is enabled by verifying read_from_replica=localize option is passed Signed-off-by: Praveen M <m.praveen@ibm.com>

Pull request has been modified.

Rakshith-R · 2023-09-26T04:51:17Z

@iPraveenParihar you can use https://docs.mergify.com/commands/rebase to rebase, It'll preserve the approvals.

iPraveenParihar · 2023-09-26T04:57:38Z

@Mergifyio queue

mergify · 2023-09-26T04:57:41Z

queue

❌ Command disallowed due to command restrictions in the Mergify configuration.

sender-permission>=write

Rakshith-R · 2023-09-26T04:58:37Z

@Mergifyio queue

mergify · 2023-09-26T04:58:40Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 6719d64

ceph-csi-bot · 2023-09-26T04:58:59Z

/test ci/centos/k8s-e2e-external-storage/1.28

ceph-csi-bot · 2023-09-26T04:58:59Z

/test ci/centos/k8s-e2e-external-storage/1.26

ceph-csi-bot · 2023-09-26T04:59:00Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2023-09-26T04:59:00Z

/test ci/centos/mini-e2e-helm/k8s-1.28

ceph-csi-bot · 2023-09-26T04:59:00Z

/test ci/centos/mini-e2e-helm/k8s-1.26

ceph-csi-bot · 2023-09-26T04:59:01Z

/test ci/centos/mini-e2e/k8s-1.28

ceph-csi-bot · 2023-09-26T04:59:01Z

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2023-09-26T04:59:01Z

/test ci/centos/k8s-e2e-external-storage/1.27

ceph-csi-bot · 2023-09-26T04:59:02Z

/test ci/centos/mini-e2e/k8s-1.26

ceph-csi-bot · 2023-09-26T04:59:02Z

/test ci/centos/mini-e2e-helm/k8s-1.27

ceph-csi-bot · 2023-09-26T04:59:03Z

/test ci/centos/mini-e2e/k8s-1.27

mergify bot added the component/deployment Helm chart, kubernetes templates and configuration Issues/PRs label Sep 7, 2023

Rakshith-R reviewed Sep 7, 2023

View reviewed changes

Madhu-1 reviewed Sep 7, 2023

View reviewed changes

iPraveenParihar force-pushed the read-affinity/e2e-and-helm branch from daea852 to cfaf65c Compare September 13, 2023 11:35

iPraveenParihar marked this pull request as ready for review September 13, 2023 13:25

iPraveenParihar force-pushed the read-affinity/e2e-and-helm branch 4 times, most recently from 0e4176a to d1d3627 Compare September 14, 2023 08:08

iPraveenParihar commented Sep 14, 2023

View reviewed changes

iPraveenParihar force-pushed the read-affinity/e2e-and-helm branch 2 times, most recently from 78b0ab9 to 2cdf37f Compare September 14, 2023 11:21

iPraveenParihar force-pushed the read-affinity/e2e-and-helm branch 3 times, most recently from feee4ed to ae0d73b Compare September 19, 2023 04:01

Madhu-1 previously approved these changes Sep 25, 2023

View reviewed changes

Rakshith-R previously approved these changes Sep 25, 2023

View reviewed changes

iPraveenParihar added 2 commits September 26, 2023 09:53

helm: add option to enable read affinity for rbd

9614bdd

This commit adds --enable-read-affinity flag to enable read affinity for rbd Signed-off-by: Praveen M <m.praveen@ibm.com>

e2e: added test to verify read affinity functionality

64e8736

e2e test case is added to test if read affinity is enabled by verifying read_from_replica=localize option is passed Signed-off-by: Praveen M <m.praveen@ibm.com>

iPraveenParihar force-pushed the read-affinity/e2e-and-helm branch from c07d242 to 64e8736 Compare September 26, 2023 04:23

Rakshith-R approved these changes Sep 26, 2023

View reviewed changes

Rakshith-R requested a review from Madhu-1 September 26, 2023 04:51

Madhu-1 approved these changes Sep 26, 2023

View reviewed changes

mergify bot added the ok-to-test Label to trigger E2E tests label Sep 26, 2023

ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Sep 26, 2023

mergify bot merged commit 6719d64 into ceph:devel Sep 26, 2023
23 checks passed

Madhu-1 mentioned this pull request Nov 3, 2023

Add E2E test cases for rbd map options. #3082

Closed

		{{- if .Values.readAffinity.enabled }}
		- "--enable-read-affinity=true"

helm & e2e: add option to enable read affinity for rbd #4111

helm & e2e: add option to enable read affinity for rbd #4111

Conversation

iPraveenParihar commented Sep 7, 2023 • edited Loading

Describe what this PR does

Closes: #3642

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rakshith-R Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iPraveenParihar commented Sep 14, 2023

iPraveenParihar commented Sep 14, 2023

iPraveenParihar commented Sep 14, 2023

iPraveenParihar commented Sep 14, 2023

iPraveenParihar commented Sep 14, 2023

iPraveenParihar commented Sep 14, 2023

iPraveenParihar commented Sep 14, 2023 • edited Loading

Rakshith-R commented Sep 15, 2023

iPraveenParihar commented Sep 19, 2023

iPraveenParihar commented Sep 19, 2023

iPraveenParihar commented Sep 19, 2023

iPraveenParihar commented Sep 19, 2023

iPraveenParihar commented Sep 19, 2023

iPraveenParihar commented Sep 19, 2023

iPraveenParihar commented Sep 19, 2023

Rakshith-R commented Sep 26, 2023

iPraveenParihar commented Sep 26, 2023

mergify bot commented Sep 26, 2023

❌ Command disallowed due to command restrictions in the Mergify configuration.

Rakshith-R commented Sep 26, 2023

mergify bot commented Sep 26, 2023 • edited Loading

✅ The pull request has been merged automatically

ceph-csi-bot commented Sep 26, 2023

ceph-csi-bot commented Sep 26, 2023

ceph-csi-bot commented Sep 26, 2023

ceph-csi-bot commented Sep 26, 2023

ceph-csi-bot commented Sep 26, 2023

ceph-csi-bot commented Sep 26, 2023

ceph-csi-bot commented Sep 26, 2023

ceph-csi-bot commented Sep 26, 2023

ceph-csi-bot commented Sep 26, 2023

ceph-csi-bot commented Sep 26, 2023

ceph-csi-bot commented Sep 26, 2023

iPraveenParihar commented Sep 7, 2023 •

edited

Loading

Rakshith-R Sep 7, 2023 •

edited

Loading

iPraveenParihar commented Sep 14, 2023 •

edited

Loading

mergify bot commented Sep 26, 2023 •

edited

Loading